Add CollectionIndexOnNonIndexedSeq rule to Scapegoat#158
Add CollectionIndexOnNonIndexedSeq rule to Scapegoat#158JoshRosen wants to merge 1 commit intoscapegoat-scala:masterfrom
Conversation
|
(Sorry, meant to open this against my own fork instead) |
|
I could add it anyway if you don't mind ? |
|
@sksamuel, my original thought was that this inspection would be too noisy for folks who aren't writing performance-intensive code. Even in Spark itself, this rule flags many cases which aren't serious performance problems due to small collections sizes, small number of calls, etc. I could see this inspection being pretty annoying for folks who aren't writing performance-sensitive imperative code in Scala. That said, we already have the opposite experience with the |
|
If you do want to accept this rule, I'll re-open and add it to the README. |
|
|
||
| override def inspect(tree: Tree): Unit = { | ||
| tree match { | ||
| case Apply(Select(lhs, TermName("apply")), List(_)) if isSeq(lhs) && !isIndexedSeq(lhs) => |
There was a problem hiding this comment.
It's probably worth ignoring the apply(0) / apply(constant) case (like I did in my linter implementation.
|
I totally messed this up by force-pushing to a closed PR branch, so let me re-open. Sorry for the noise :( |
2 participants
Inspired by https://twitter.com/jshrsn/status/865677863646658560, this patch adds a new
IndexingOnANonIndexedSeqrule which raises a warning when we see code which indexes on aSeqwhich isn't known to be anIndexedSeqsubtype. The goal here is to make it easier to prevent cases where someone writes anO(n^2)loop by accident.This rule should have prevented the following Spark bugs:
For comparision, here's the same rule implemented in
linter: JoshRosen/linter#1